-
Notifications
You must be signed in to change notification settings - Fork 337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proof-of-concept serialization of advanced JSON types, records #1073
Proof-of-concept serialization of advanced JSON types, records #1073
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is really nice, thanks taking on this work!
Just a few nitpicks and you'll need to fix the DCO check by signing off on your commit (e.g. git commit -s
).
Codecov Report
@@ Coverage Diff @@
## master #1073 +/- ##
==========================================
- Coverage 67.02% 66.45% -0.58%
==========================================
Files 170 171 +1
Lines 5693 5750 +57
Branches 607 624 +17
==========================================
+ Hits 3816 3821 +5
- Misses 1732 1782 +50
- Partials 145 147 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed nits
c2c3523
to
1ec0dc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@onionhammer Can you please rebase this PR to master? It seems to be having cherry-picked master commits. Instead of that, rebasing could be more appropriate.
@shivamkm07 My git fu is weak, so please let me know if what I did there helps |
@onionhammer All the extra commits are still there. Seems you have cherry-picked master commits to your branch to update. You must not do that. Instead just merge/rebase master to update the changes. To fix the PR, run the following commands:
Note that above I have used commit hash of the three original commits in the PR. Please ensure these are the ones with the changes you want . Also do re-check everything before doing |
892c377
to
8d25615
Compare
@shivamkm07 ok, how about now? |
Good now. Thanks for updating the PR. |
@onionhammer can you please fix the broken test? One of the integration test is failing |
@shivamkm07 To be honest, I'm looking at this now, and I'm simply unable to reproduce the error that the CI is getting, any help investigating this would be much appreciated. I'm available on discord today if anyone wants to ping me about it. "error invoke actor method: failed to cast response" is an error I get in production dapr actors occasionally, this branch aside. Even on |
I would love to get some assistance on this one if anyone has the time; I'm available on discord - IM me |
@shivamkm07 still unable to reproduce any issues with the tests locally. Is there a way to run the github actions locally (in a way that works?) Act doesnt seem to work for this project. |
Tests seem to pass after the changes. Thanks for the fix. Please also fix the DCO. |
9593aaa
to
cc0c219
Compare
Signed-off-by: Erik O'Leary <[email protected]>
Signed-off-by: Erik O'Leary <[email protected]>
…predictable Signed-off-by: Erik O'Leary <[email protected]>
cc0c219
to
e328243
Compare
Following up here - just merged a PR that'll deploy with 1.15 in which I've addressed the JSON serialization alongside the existing Data Contract serialization. |
Description
Today, complex objects being passed to/from dapr actors must be serializable with DataContractSerializer, which means shotgunning "DataMember" on any primary-constructors (records and C# 8 classes), and difficult workarounds types such as JsonElement.
This PR is a proof-of-concept, as there are areas where this implementation is compromised. A deeper rewrite of Dapr serialization (while still maintaining backwards compatibility) is still necessary.
Issue reference
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: